livekit_bridge: An ergonomic interface into the Livekit C++ SDK#58
livekit_bridge: An ergonomic interface into the Livekit C++ SDK#58stephen-derosa wants to merge 9 commits intolivekit:mainfrom
Conversation
|
|
xianshijing-lk
left a comment
There was a problem hiding this comment.
Good work, that is a large PR, I will need more time to review it.
some initial questions to get a clearer picture on the design.
|
|
||
| // Check for a new video frame | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_latest_video.mutex); |
There was a problem hiding this comment.
can we try avoiding locking while doing the heavy duty things ?
I think this can be achieved by having a local_pixel, and just swap the buffer, like
std::vectorstd::uint8_t local_pixels;
int fw = 0, fh = 0;
bool have_frame = false;
{
std::lock_guardstd::mutex lock(g_latest_video.mutex);
if (g_latest_video.dirty && g_latest_video.width > 0 && g_latest_video.height > 0) {
fw = g_latest_video.width;
fh = g_latest_video.height;
local_pixels.swap(g_latest_video.data); // avoid copy
g_latest_video.dirty = false;
have_frame = true;
}
}
and use the local_pixel and other local variables for rendering.
There was a problem hiding this comment.
great point -- updated in the latest code!
bridge/src/livekit_bridge.cpp
Outdated
There was a problem hiding this comment.
In general, I think it is not recommended to call Connect() while holding the std::lock_guardstd::mutex lock(mutex_);
maybe it is better to create a local room, like
auto room = std::make_uniquelivekit::Room();
set things up, then reassign it to room_
There was a problem hiding this comment.
updated via constructing the room, then acquiring the lock and std:move-ing to the member var. Also added a connecting_ flag to guard against connect() being called impatiently/improperly by developers
| * @param token Access token for authentication. | ||
| * @return true if connection succeeded. | ||
| */ | ||
| bool connect(const std::string &url, const std::string &token); |
There was a problem hiding this comment.
curiously, don't you need to expose the room option via the connect() function ?
There was a problem hiding this comment.
My experimentation shows me that you dont actually need to specify the room, which i think makes sense since the token should only be valid for a single room. @ladvoc can you confirm if the FFI needs a room, or if the url and token themselves are enough?
| * @param source Track source (e.g. SOURCE_MICROPHONE). | ||
| * @param callback Function to invoke per audio frame. | ||
| */ | ||
| void registerOnAudioFrame(const std::string &participant_identity, |
There was a problem hiding this comment.
is it register** the right word ?
I wonder what will happen if users register the same callback or multiple callbacks here ?
do we support those use cases ?
if not, can we make it a bit less ambiguous ?
There was a problem hiding this comment.
"register*" has been a pattern i have seen in the robotics space, but i see the confusion. I dont think we should support registering multiple callbacks for a single stream/track since IMO it adds necessary complexity, but i could be convinced otherwise.
I can rename to setOnAudioFrameCallback() to be more explicit?
| * If a reader thread is active for this (identity, source), it is | ||
| * stopped and joined. | ||
| */ | ||
| void unregisterOnAudioFrame(const std::string &participant_identity, |
There was a problem hiding this comment.
it looks like it should "clear" rather than unregister ?
There was a problem hiding this comment.
if we agree on only one callback per stream/track, then 100% i agree this should be clearOnAudioFrameCallback()
There was a problem hiding this comment.
hmm. this is a bit worrying, are we saying that we will have multiple render threads if we want to render multiple tracks ?
There was a problem hiding this comment.
the *Reader() just handle getting audio/video frames -- it doesnt actually do any rendering/playback of said frames. These start*Reader() functions take in a callback which will get if a frame is received. Said callbacks will handle rendering/processing/etc.
| * mic->mute(); | ||
| * mic.reset(); // unpublishes | ||
| */ | ||
| class BridgeAudioTrack { |
There was a problem hiding this comment.
Are these BridgeAudioTrack, BridgeVideoTrack public interface to our developers ?
Note, from the code, these track implementations are not thread safe on its own, that says, if users want to get access to these bridge track, they are not protected
There was a problem hiding this comment.
I added a mutex to bridge_audio_track.h and bridge_video_track.h to ensure thread safety -- thanks you! This was a big miss!
| released_ = true; | ||
|
|
||
| // Unpublish the track from the room | ||
| if (participant_ && publication_) { |
There was a problem hiding this comment.
From what I read, the BridgeVideoTrack can outlive the LivekitBridge::disconnect(), like that can be destruct after calling livekitBridge::disconnect().
In that case, how does our code guarantee that the it will not call into the participant_ ?
There was a problem hiding this comment.
this is a good point i hadnt considered. I think we can handle it in a few ways:
- we store std::weak_ptrs to all the tracks in the livekit_bridge. When the livekit_bridge calls disconnect(), it steps through all tracks and calls release().
- the bridge could pass each track a atomic bool which gets set when the livekit_bridge calls disconnect(). Then the track checks said bool in its control flow.
I think we will likely want to do things to the tracks in the future, like maybe mute/unmute through a more generic livekit_bridge public function (like muteAllAudio()) which might justify using weak_ptrs being stored in the bridge, but im not sure what the correct approach is here. What is your though?
bridge/src/livekit_bridge.cpp
Outdated
There was a problem hiding this comment.
nit, should you add an assert to make sure room_ is nullptr here ?
There was a problem hiding this comment.
great point -- added assert here and when we std::make_unique<BridgeRoomDelegate>
| { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
|
|
||
| if (!connected_) { |
There was a problem hiding this comment.
I think it is wrong to return here.
For instance, if the connect() go thru the steps and fail at bool result = room_->Connect(url, token, options);
the connected_ is false, but you never call livekit::shutdown(); to clear up things
There was a problem hiding this comment.
great catch -- i replaced the return with:
std::cerr << "[LiveKitBridge] Attempting to disconnect an already "
"disconnected bridge. Things may not disconnect properly.\n";
…s. livekit_bridge.cpp: disconnect() if cerr, but continue with disconnect
…ing_ flag to guard against calling connect() more than once
instead of calling stopThread() before creating, we do cleanup after calling start.
examples/robot.cpp for sends webcam/mic and sim video/audio feeds to the human participant. examples/human.cpp takes in human input to switch between real/sim streams
1aa8e12 to
098e1f4
Compare
Overview
An ergonomic library providing simple usage of the C++ SDK.
Building
This library is attached to the build system of the core C++ SDK library. Use
build.shas is.Testing
The
bridge/examples/directory has simulatedhumanandrobottests. There are 4 files:These have been tested manually.
Unit tests
CallbackKeyhashing/equality,BridgeAudioTrack/BridgeVideoTrackstate management, andLiveKitBridgepre-connection behaviour (callback registration, error handling).Limitations
The bridge is designed for simplicity and currently only supports limited audio and video features. It does not expose:
RoomOptionsorTrackPublishOptionsFor advanced use cases, use the full
client-sdk-cppAPI directly, or expand the bridge to support your use case.TODOs before merge